Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Dashboard] EUI Visual Refresh Integration #204442

Merged
merged 8 commits into from
Dec 18, 2024
Merged

[Dashboard] EUI Visual Refresh Integration #204442

merged 8 commits into from
Dec 18, 2024
+45 −42

Conversation

cqliu1
Copy link
Contributor

@cqliu1 cqliu1 commented Dec 16, 2024 •
edited
Loading

Summary

Related to #203132.

Part of #204590.

This replaces all references to euiThemeVars in favor of the useEuiTheme hook in the dashboard plugin. This also updates the color of the presentational panel border in edit mode from mediumShade, which was deprecated, to BorderBaseFormsControl as the darker border color.

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

@cqliu1 cqliu1 force-pushed the borealis/dashboard/replace-euiThemeVars-usage branch from a63e9e5 to fd9b122 Compare December 16, 2024 16:49
@cqliu1 cqliu1 added Feature:Dashboard Dashboard related features EUI Visual Refresh loe:medium Medium Level of Effort labels Dec 16, 2024
@cqliu1 cqliu1 force-pushed the borealis/dashboard/replace-euiThemeVars-usage branch from bb1f68c to bddbc7d Compare December 16, 2024 17:05
@cqliu1 cqliu1 force-pushed the borealis/dashboard/replace-euiThemeVars-usage branch from bddbc7d to 6d69138 Compare December 16, 2024 17:08
@cqliu1 cqliu1 mentioned this pull request Dec 16, 2024
Closed
9 tasks
mgadewoll
mgadewoll reviewed Dec 17, 2024
View reviewed changes
.../presentation_panel/public/panel_component/panel_header/presentation_panel_hover_actions.tsx Outdated Show resolved Hide resolved
.../presentation_panel/public/panel_component/panel_header/presentation_panel_hover_actions.tsx Outdated
@@ -142,11 +147,11 @@ export const PresentationPanelHoverActions = ({
const hoverActionsWidth =
(rightHoverActionsRef.current?.offsetWidth ?? 0) +
(dragHandleRef.current?.offsetWidth ?? 0) +
parseInt(euiThemeVars.euiSize, 10) * 2;
parseInt(euiTheme.size.m, 10) * 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be euiTheme.size.base as euiSize was 16px and euiTheme.size.m is 12px
(same for the following occurrences)

📖 size docs, migration overview

.../presentation_panel/public/panel_component/panel_header/presentation_panel_hover_actions.tsx Outdated Show resolved Hide resolved
.../presentation_panel/public/panel_component/panel_header/presentation_panel_hover_actions.tsx Outdated
position: absolute;
top: 0;
display: flex;
justify-content: space-between;
padding: 0 ${euiThemeVars.euiSize};
padding: 0 ${euiTheme.size.medium};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking euiTheme.size.medium does not exists. Based on the original value, this likely should be euiTheme.size.base.

@cqliu1 cqliu1 changed the title [Dashboard][Borealis] Replace eui theme vars usage [Dashboard] EUI Visual Refresh Integration Dec 17, 2024
@cqliu1 cqliu1 added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas impact:critical This issue should be addressed immediately due to a critical level of impact on the product. v9.0.0 labels Dec 17, 2024
@cqliu1 cqliu1 marked this pull request as ready for review December 17, 2024 18:53
@cqliu1 cqliu1 requested a review from a team as a code owner December 17, 2024 18:53
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@cqliu1 cqliu1 added backport:version Backport to applied version labels release_note:skip Skip the PR/issue when compiling release notes labels Dec 17, 2024
nreese
nreese reviewed Dec 17, 2024
View reviewed changes
.../presentation_panel/public/panel_component/panel_header/presentation_panel_hover_actions.tsx

const { euiTheme } = useEuiTheme();

const EDIT_MODE_OUTLINE = `${euiTheme.border.width.thin} dashed ${euiTheme.colors.borderBaseFormsControl}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use useMemo to avoid recreating these string on each render.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think useMemo usage is best left to things which are expensive to compute. useMemo itself can introduce overhead .

mgadewoll
mgadewoll approved these changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Changes LGTM from EUI side

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #12 / UserCommentPropertyActions renders the correct number of actions

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
presentationPanel 15.8KB 15.6KB -132.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
presentationPanel 44.0KB 43.9KB -33.0B

History

@cqliu1 cqliu1 merged commit 4a6e1e5 into elastic:main Dec 18, 2024
8 checks passed
@cqliu1 cqliu1 mentioned this pull request Dec 18, 2024
Open
10 tasks
JoseLuisGJ pushed a commit to JoseLuisGJ/kibana that referenced this pull request Dec 19, 2024
@cqliu1 @kibanamachine @JoseLuisGJ
[Dashboard] EUI Visual Refresh Integration (elastic#204442)
5aa2fbf 
## Summary

Related to elastic#203132.
Closes elastic#204590.

This replaces all references to euiThemeVars in favor of the useEuiTheme
hook in the dashboard plugin. This also updates

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 20, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 204442 locally

2 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 204442 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 204442 locally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThomThomson ThomThomson ThomThomson left review comments

@mgadewoll mgadewoll mgadewoll approved these changes

@nreese nreese nreese approved these changes

Assignees
No one assigned
Labels
backport missing Added to PRs automatically when the are determined to be missing a backport. backport:version Backport to applied version labels EUI Visual Refresh Feature:Dashboard Dashboard related features impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cqliu1 @elasticmachine @kibanamachine @nreese @ThomThomson @mgadewoll